Skip to content

Add experimental async transport #4572

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: potel-base
Choose a base branch
from

Conversation

srothh
Copy link
Member

@srothh srothh commented Jul 11, 2025

Copy link

codecov bot commented Jul 11, 2025

❌ 17 Tests Failed:

Tests completed Failed Passed Skipped
20854 17 20837 1098
View the top 3 failed test(s) by shortest run time
tests.integrations.redis.test_redis::test_data_truncation_custom
Stack Traces | 0.088s run time
.../integrations/redis/test_redis.py:205: in test_data_truncation_custom
    connection = FakeStrictRedis()
.tox/py3.11-redis-v4/lib/python3.11.../site-packages/fakeredis/_connection.py:185: in __init__
    super().__init__(**kwds)
E   TypeError: Redis.__init__() got an unexpected keyword argument 'lib_name'
tests.integrations.redis.test_redis::test_redis_pipeline[True-True-expected_first_ten1]
Stack Traces | 0.088s run time
.../integrations/redis/test_redis.py:62: in test_redis_pipeline
    connection = FakeStrictRedis()
.tox/py3.11-redis-v4/lib/python3.11.../site-packages/fakeredis/_connection.py:185: in __init__
    super().__init__(**kwds)
E   TypeError: Redis.__init__() got an unexpected keyword argument 'lib_name'
tests.integrations.redis.test_redis_cache_module::test_no_cache_basic
Stack Traces | 0.088s run time
.../integrations/redis/test_redis_cache_module.py:26: in test_no_cache_basic
    connection = FakeStrictRedis()
.tox/py3.11-redis-v4/lib/python3.11.../site-packages/fakeredis/_connection.py:185: in __init__
    super().__init__(**kwds)
E   TypeError: Redis.__init__() got an unexpected keyword argument 'lib_name'

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@srothh srothh changed the title ref(transport): Added shared sync/async transport superclass and created a sync http subclass ref(transport): Add shared sync/async transport superclass and create a sync http subclass Jul 11, 2025
@srothh srothh marked this pull request as ready for review July 11, 2025 12:16
@srothh srothh requested a review from a team as a code owner July 11, 2025 12:16
@srothh srothh marked this pull request as draft July 11, 2025 12:17
@srothh srothh marked this pull request as ready for review July 11, 2025 12:17
@srothh srothh marked this pull request as draft July 17, 2025 09:43
@srothh srothh force-pushed the srothh/transport-class-hierarchy branch from 53b92f2 to 3607d44 Compare July 21, 2025 09:48
@srothh srothh marked this pull request as ready for review July 22, 2025 10:02
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than my little nitpicking, this looks great!

srothh added 10 commits July 28, 2025 10:48
…ted a sync transport HTTP subclass

Moved shared sync/async logic into a new superclass (HttpTransportCore), and moved sync transport specific code into a new subclass(BaseSyncHttpTransport), from which the current transport implementations inherit

Fixes GH-4568
Removed an unnecessary TODO message and reverted a class name change for BaseHTTPTransport.

GH-4568
Adds test coverage for the error handling path when HTTP requests return
error status codes.

GH-4568
Restore comments accidentally removed during a previous commit.
Refactored class names such that BaseHttpTransport now has the same functionality as before the hierarchy refactor

GH-4568
Add a new flush_async method in the Transport ABC. This is needed for the async transport, as calling it from the client
while preserving execution order in close will require flush to be a coroutine, not a function.

GH-4568
Move flush_async down to the specific async transport subclass. This makes more sense anyway, as
this will only be required by the async transport. If more async transports are expected,
another shared superclass can be created.

GH-4568
Add necessary type annotations to the core HttpTransport to accomodate for async transport.

GH-4568
@srothh srothh force-pushed the srothh/transport-class-hierarchy branch from 062ab5b to 4e56e5c Compare July 28, 2025 08:48
Copy link
Member

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Aug 12, 2025

okay, so now that we have 2 base/core classes, it's a bit more confusing than before what the contracts of each entity actually are.

I want us to define more clearly and explicitly what is

  • shared across sync and async
  • only sync
    • only http2
  • only async

but I think it's clearer if we merge all the changes in first and then do some shuffling. We will do that with mixin classes.
https://adamj.eu/tech/2025/05/01/python-type-hints-mixin-classes/

srothh and others added 2 commits August 12, 2025 15:13
…orker (#4580)

Add a new abstract base class for the background worker implementations
in the transport. This allows for implementation of the upcoming async
task-based worker in addition to the current thread based worker used in
the sync context. Additionally, add a factory method in the
HttpTransportCore shared class, to allow the worker methods to live
higher up in the class hierarchy regardless of specific implementation.

GH-4578
Add a new implementation of the transport background worker based on an
async task. This worker mostly mirrors the same functionality as the
thread-based worker, with the exception that it exposes a non-blocking
async flush (which can be awaited from an async context). Furthermore,
the worker itself is not thread-safe and should be called using
[run_coroutine_threadsafe](https://docs.python.org/3/library/asyncio-task.html#asyncio.run_coroutine_threadsafe)
or similar when called from another thread (this is fixed and handled by
the transport). I have kept the fork check from the threaded worker, but
I am not sure if it is necessary as forking in an async application
would also break the event loop.

GH-4581

---------

Co-authored-by: Neel Shah <[email protected]>
cursor[bot]

This comment was marked as outdated.

srothh and others added 2 commits August 13, 2025 18:21
Add async implementation of the abstract Transport class. This transport
utilizes the async task worker as well as the httpcore async
functionality.

Thread Safety: As capture_envelope is registered by the client as a
callback for several background threads in the sdk, which are not
running the event loop, capture_envelope in the transport is made to be
thread safe and allow for execution on the event loop from other
threads. The same is currently not the case for flush, as there does not
seem to be a usage from background threads, however if necessary, it can
also be added.

HTTP2 support: Currently not activated, but from the looks of the
[httpcore docs](https://www.encode.io/httpcore/http2/) it should be as
simple as setting the http2 in the init of the pool to true. This likely
makes sense to support, as HTTP2 shows great performance improvements
with concurrent requests.

Kill: The kill method is sync, but the pool needs to be closed
asynchronously. Currently, this is done by launching a task. However,
the task cannot be awaited in sync code without deadlocking, therefore
kill followed by an immediate loop shutdown could technically lead to
resource leakage. Therefore, I decided to make kill optionally return
the async task, so it can be awaited if called from an async context.

Note also that parts of the code are very similar to the HTTP2
integration, as they both use the httpcore library. Maybe in a later PR
there could be a shared superclass to avoid code duplication?

GH-4582

---------

Co-authored-by: Neel Shah <[email protected]>
Integrate the async transport with the rest of the SDK. Provide a new
experimental option `transport_async` that enables the async transport
if an event loop is running. Otherwise, fall back to the sync transport.

Furthermore, adapt the client to work with the async transport. To this
end, flush and close were changed to be non blocking and awaitable in an
async context to avoid deadlocks, however close enforces a completed
flush before shutdown. As there are to my knowledge no background
threads running flush/close, these methods are currently not
thread-safe/loop-aware for async, which can be changed if necessary.

Atexit issue: The atexit integration used by the SDK runs after the
event loop has already closed if asyncio.run() is used. This makes it
impossible for the async flush to happen, as atexit calls
client.close(), but a loop is no longer present. I attempted to apply
[this fix](https://discuss.python.org/t/atexit-for-asyncio/13911) by
patching the loop close in the asyncio integration, but I am unsure if I
did it correctly/put it in the correct spot, or if this is a good idea.
From my SDK test however, it seems to fix the flush issue. Note also
that this will apparently be patched in Python 3.14, as per the
discussion in the linked thread.

As a final note, I added event loop checking. Whenever the event loop is
used, the transport/client catch RuntimeErrors, which would be thrown in
case the event loop was already shut down. I am not sure if this is a
case we need to consider, but I added it for now because I did not want
the transport to potentially throw RuntimeError if the event loop is
shutdown during a program. If this should be left out currently for
simplicity, I can remove it again.

I added the [httpcore[asyncio] ](https://www.encode.io/httpcore/async/)
dependency to requirements-testing, as it is needed for the async
httpcore functionality.

GH-4601

---------

Co-authored-by: Neel Shah <[email protected]>
@sl0thentr0py sl0thentr0py changed the title ref(transport): Add shared sync/async transport superclass and create a sync http subclass Add experimental async transport Aug 14, 2025
cursor[bot]

This comment was marked as outdated.

#4700)

Filter SDK internal tasks from span creation. Implements a new
contextmanager for tasks spawned internally by the SDK, which filters it
in the asyncio integration.

GH-4699

---------

Co-authored-by: Neel Shah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants